feat: case insensitive diff_test and fix manifest bug #527
Conversation
peakschris
requested review from
brandjon,
c-mita,
comius,
hvadehra and
tetromino
as code owners
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
peakschris
changed the title
There are four issues when running on windows: 1. #330 2. #331 3. #332 4. #333 This PR fixes 3 and 4. There are related PRs in bazel-skylib and bazel-starlib. There is no dependency -- the PRs can close in any order. - cgrindel/bazel-starlib#446 (fixes 1) - bazelbuild/bazel-skylib#527 (fixes 2) ### Test results: Before: --enable_runfiles: 0 pass --noenable_runfiles: 0 pass After this PR: --enable_runfiles: 52 pass, 19 failures --noenable_runfiles: 51 pass, 20 failures (17 are doc diff-tests due to bazel-starlib) After this PR, together with wip PRs for 1 and 2: --enable_runfiles: 71 pass, 0 failures --noenable_runfiles: 51 pass, 20 failures (all due to bazel-starlib)
|
@brandjon @tetromino @comius @hvadehra @c-mita gentle ping, would it be possible for someone to review this? I have a chain of PRs to rulesets dependent on this one to improve windows support. |
|
@peakschris Did you see this message? This may not be reviewed and merged. I will bring this up at the next Community for Bazel technical steering committee meeting which is scheduled for Tuesday, July 9. |
| def _ignore_line_endings(ctx): | ||
| ignore_line_endings = "0" | ||
| if ctx.attr.ignore_line_endings: | ||
| ignore_line_endings = "1" | ||
| return ignore_line_endings | ||
|
|
There was a problem hiding this comment.
Instead of a func for this, I'd opt for a one-liner on its call site. This way its a bit more readable, because I don't need to jump around file, to see what it does.
e.g. "1" if ctx.attr.ignore_line_endings else "0"
| set "TR=C:\\msys64\\usr\\bin\\tr.exe" | ||
| ) | ||
| if not exist !TR! ( | ||
| echo>&2 WARNING: ignore_line_endings set but !TR! not found; line endings will be compared |
There was a problem hiding this comment.
Bazel team is quite opinionated about warnings. This should be an error.
|
|
||
| def _diff_test_impl(ctx): | ||
| if ctx.attr.is_windows: | ||
| bash_bin = ctx.toolchains["@bazel_tools//tools/sh:toolchain_type"].path |
There was a problem hiding this comment.
Now that we require bash, I wonder if the maintenance of this rule would be simpler, to use the same bash script on windows (and maybe fallback to fc or just use diff)?
|
|
||
| (cd "$ws" && \ | ||
| bazel test ${flags} "//${subdir%/}:same" --test_output=errors 1>"$TEST_log" 2>&1 \ | ||
| bazel test ${flags} "//${subdir%/}:same" --test_output=errors --noshow_progress 1>>"$TEST_log" 2>&1 \ |
There was a problem hiding this comment.
Why are you changing tests?
| if ! diff {strip_trailing_cr}"$RF1" "$RF2"; then | ||
| MSG={fail_msg} | ||
| if [[ "${{MSG}}" == "" ]]; then | ||
| MSG="why? diff {strip_trailing_cr}"${{RF1}}" "${{RF2}}" | cat -v" |
There was a problem hiding this comment.
Why is this message so different from the message before? Also use of RF1 is inconsistend with use of {file1} below.
comius
left a comment
comius
left a comment
There was a problem hiding this comment.
I think this is quite complicated change and this brings with it potentially higher mainteinance costs.
There are multiple issues on windows: 1. bazelbuild/bazel-skylib#527 2. #448 3. #449 4. #447 5. #450 Issues 2-5 above are all fixed by this PR ### Test results (windows) (still filling in these results) Before --enable_runfiles 332 failures --noenable_run With buildifier_prebuilt update only --enable_runfiles 116 failures --noenable_run 154 failures + skips With this PR only --enable_runfiles 34 failures --noenable_runfiles 122 failures + skips With this PR and bazelbuild/bazel-skylib#527 --enable_runfiles 0 failures --noenable_runfiles 85 failures ### Future activity Note: most of the --noenable_runfiles failures are of this form. It seems that bzlformat_lint_test.sh is designed to require runfiles. ``` INFO: From Testing //tests/shlib_tests/lib_tests/files_tests:bzlformat_lint_test: ==================== Test output for //tests/shlib_tests/lib_tests/files_tests:bzlformat_lint_test: D:/udu/b/356umzpb/execroot/_main/bazel-out/x64_windows-fastbuild/bin/tests/shlib_tests/lib_tests/files_tests/bzlformat_lint_test.sh: line 14: tests/shlib_tests/lib_tests/files_tests/bzlformat_lint_test_BUILD.bazel.sh: No such file or directory tests/shlib_tests/lib_tests/files_tests/bzlformat_lint_test_BUILD.bazel.sh failed with 127. 1 lint tests failed. ================================================================================ ``` Look into this: ``` INFO: From Action tests/bzlrelease_tests/rules_tests/generate_workspace_snippet_tests/archive_sha256: which: no shasum in (/usr/bin:/usr/bin:/bin:/c/WINDOWS:/c/WINDOWS/System32:/c/WINDOWS/System32/WindowsPowerShell/v1.0) ``` --------- Co-authored-by: Chuck Grindel <[email protected]>
3 participants
In my work on windows quality in rulesets, a frequent issue I am finding is that diff_tests are failing due to differences in line endings -- either due to differences in the 'expected' values, or cross-platform differences in generators (e.g. buildifier always writes LF, whilst skydoc writes LF or CRLF depending on platform).
We can't expect rule authors to work around these differences, because in practice they do not have good access to windows dev hardware.
The goal of this PR is to make diff_test slightly less brittle and ignore line endings by default. An
ignore_line_endingsflag is provided to switch this behavior.This flag is supported by the
diffcommand on unix. On windows, it requires thetr.execommand that is part of most bash installs including msys2. Iftr.execannot be found theignore_line_endingsflag is silently ignored and set to False.This PR allows us to enable the 1 disabled test 'stardoc_with_diff_test' on windows CI, and also fixes many test failures in dependent rulesets such as rules_starlib and rules_bazel_integration_test